descr: catch the field-position invariant where it is produced, not where it is repaired by youknowone · Pull Request #1053 · youknowone/pyre

youknowone · 2026-08-05T08:49:03Z

Summary

A field descr's index_in_parent (descr.py:228) must name the slot its parent
puts the field in — upstream gets this for free because heaptracker.py:60-72 get_fielddescr_index_in and :96-112 all_fielddescrs are one walker sharing one
skip set. pyre has several independent producers, and
all_fielddescrs()[index_in_parent] is a load-bearing lookup
(optimizeopt/info.rs force_box), so a stale rank either runs off the end or
emits the store against a different field.

Two producers were leaving that rank stale, and the counter that looks like it
guards this could not see either of them.

The counter could not have caught this

field_pos_rederived was documented as "the one to watch". It cannot be:

GcCache::derive_index_in_parent is the judge and the repairman — it
replaces the caller's number with the parent's in the same expression, so a
nonzero reading is a log of repairs already applied, never a defect still
present.
It mostly never asks. make_simple_descr_group_keyed_with_headerless mints
every field before register_keyed_size publishes the parent those fields
will be indexed against, so the dominant path takes the FIELD_PARENT_ABSENT
early return. Measured on this tree: parent_absent 1202 against rederived
0.

General shape, worth keeping: a counter placed inside a normalising reader
measures repairs, not defects, over whatever subset the reader chose to examine.

What replaces it

Two censuses taken before anything can normalise, each with a real denominator:

counter	what it reads	this tree
`field_pos_spec_{checked,misplaced}`	a parent's positional list as handed to either group factory, including the `cache_key == 0` groups that never enter `_cache_size`	1712 / 0
`field_pos_attached_{checked,misplaced}`	a standalone field descr against the parent its own producer attached	247 / 0

Both *_misplaced join JITSTATS_BADNESS_FIELDS. A field absent from a baseline
reads as 0, so they gate from the first run with no re-record.

…and why a census was still not enough

Negative control, fix reverted, full corpus: all four of field_pos_rederived,
field_pos_spec_misplaced, field_pos_attached_misplaced and
positional_misplaced still read 0, with attached_checked unchanged at 248.
The defect is input-order dependent — it needs a field minted before a
lower-offset sibling registers — so a corpus that registers in offset order
exercises the producer without exercising the bug. Building a better census did
not help; that is the finding.

So the load-bearing check is at the construction site.
field_descr_position_disagreement runs at try_finish under
cfg!(debug_assertions) over every emitted Field descr, covering all three
states the pass can leave behind: the offset resolves (index and name are that
slot's), the offset is absent (the (0, "") fallback is intact — those are the
inline aggregates heaptracker.py:68-69 mints no descr for), and the parent's
type_id is absent from struct_size_specs (the continue that patched
nothing). Removing the re-resolution panics with
field descr at offset 16 … claims slot 0 named "hi", but that offset is slot 1.

The two producers

jitcode/assembler.rs — patch_field_descr_parents already swapped the
snapshotted parent for the final merged spec but left index_in_parent /
name, which came from that same snapshot, so one descr's two halves
described different fields. register_struct_layout re-indexes on every
merge, which is what invalidates the snapshot.
codewriter/assembler.rs — fielddescrof's struct_layout_for and
heuristic_field_layout arms overwrite offset from a second source and
leave index_in_parent at its 0 initialiser while parent carries the
whole flattened list. The found_parent_field arm is deliberately left alone:
it picked its spec by name, and re-resolving by offset would return the wrong
one for a struct with two fields at one offset.

Both fixes are pinned by a test that constructs the state directly, each with the
revert-and-fail control shown.

wasm

The two new badness fields would have read absent-and-therefore-zero on wasm, the
exact hole the descr_set_* exports were added to close. Exported the same way.
Not redundant with the native readings — the invariant is stated in byte offsets
and wasm32 lays structs out on a 4-byte word (symbolic.py:12); the wasm run
reports its own denominators (spec_checked=1665, attached_checked=245 against
1712/247 on the dev host), so it is measuring the wasm-side universe.

Note on the first three commits

738a524 / 39bd8a9 add an executed-allocation GC root list and then patch a
crash that root itself introduced; e8204d9 removes both. They net to zero and
are kept as the record: the finding they answered ("the execute_new result
lives only in a borrowed slice that is not a GC root") is false — the result
is stamped onto the recorded op's value cell (history.py:803-807 *FrontendOp(pos, value)) and MetaInterp::walk_active_trace_refs has forwarded
those cells since d3bfd17. Say the word and I will fold them out.

Verification

check.py dynasm 378/378, cranelift 378/378, wasm 374/374;
cargo test --workspace --no-default-features --features dynasm 101 suites, no
failures; cargo fmt --all --check clean. No jitstats baseline was re-recorded.

— authored by Claude

Summary by CodeRabbit

New Features
- Added JIT field-position diagnostics with checked and misplaced counts.
- Exposed field-position metrics through runtime, WebAssembly, and JIT statistics interfaces.
- Structural regressions now report increases in field-position mismatches.
Bug Fixes
- Improved field index and name resolution for reordered or merged layouts.
- Preserved fallback behavior for ambiguous or unmatched fields.
Tests
- Added coverage for reordered, shared-offset, and zero-sized fields.
- Added allocation tests verifying recorded object references.

coderabbitai · 2026-08-05T08:49:40Z

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a8e4e270-a65d-49c5-932f-40e20e42134e

📥 Commits

Reviewing files that changed from the base of the PR and between da5e6fb and c98b31d.

📒 Files selected for processing (16)

majit/majit-ir/src/descr.rs
majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs
majit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rs
majit/majit-metainterp/src/jitcode/assembler.rs
majit/majit-metainterp/src/pyjitpl/dispatch.rs
majit/majit-metainterp/src/trace_ctx.rs
majit/majit-translate/src/codewriter/assembler.rs
pyre/check.py
pyre/pyre-jit-trace/src/descr.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
pyre/pyre-jit-trace/src/jitcode_runtime.rs
pyre/pyre-jit/src/lib.rs
pyre/pyre-wasm-runner/src/main.rs
pyre/pyre-wasm/src/lib.rs

Walkthrough

The PR adds producer-side field-position diagnostics, resolves descriptor indices by names or unique offsets, exports four counters through runtime and wasm interfaces, and adds allocation-rooting documentation and tests.

Changes

Field position diagnostics

Layer / File(s)	Summary
Producer position census `majit/majit-ir/src/descr.rs`, `majit/majit-metainterp/src/pyjitpl/dispatch.rs`, `pyre/pyre-jit-trace/src/descr.rs`	Descriptor factories and bridges record submitted field positions and attached parent indices.
Descriptor layout reconciliation `majit/majit-metainterp/src/jitcode/assembler.rs`, `majit/majit-translate/src/codewriter/assembler.rs`, `majit/majit-macros/src/jit_interp/jitcode_lower/*`, `majit/majit-metainterp/src/pyjitpl/dispatch.rs`	Field names flow through field access emitters. Descriptor resolution uses names first and unique offsets second. Debug checks and regression tests validate merged-layout behavior.
Diagnostic publishing `pyre/pyre-jit-trace/src/jitcode_runtime.rs`, `pyre/pyre-jit/src/lib.rs`, `pyre/pyre-wasm/src/lib.rs`, `pyre/pyre-wasm-runner/src/main.rs`, `pyre/check.py`	Runtime APIs, wasm exports, statistics output, and badness gates expose the four checked and misplaced counters.

Allocation rooting validation

Layer / File(s)	Summary
Allocation rooting checks `pyre/pyre-jit-trace/src/trace_ctx.rs`, `pyre/pyre-jit-trace/src/jitcode_dispatch/*`	Documentation describes allocation root lifetimes. Backend tests verify that allocation results are stamped onto recorded operations.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DescriptorFactories
  participant GcCache
  participant JitcodeRuntime
  participant WasmExports
  participant WasmRunner
  DescriptorFactories->>GcCache: Record field-position checks
  GcCache->>JitcodeRuntime: Provide checked and misplaced counts
  JitcodeRuntime->>WasmExports: Expose field_position_counts
  WasmExports->>WasmRunner: Return four counters
  WasmRunner->>WasmRunner: Emit MAJIT_STATS

Possibly related PRs

youknowone/pyre#897: Both changes modify field descriptor position handling and cache validation.
youknowone/pyre#1013: Both changes modify field_descr_ref_from_bh parent handling.
youknowone/pyre#787: Both changes modify field descriptor resolution and diagnostics.

Suggested reviewers: lifthrasiir

Poem

A rabbit checks each field in line,
Counts each slot by design.
Descriptors find their proper place,
Wasm carries counts with grace.
Roots hold allocations tight—
The JIT hops onward, right.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: detecting field-position invariant violations at their producer sites instead of repairing them later.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch gc-decouple

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ba7db5471

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-05T08:55:25Z

+            && let Some(pos) = parent_spec
+                .all_fielddescrs
+                .iter()
+                .position(|spec| spec.offset == offset)


Keep fallback fields from borrowing sibling slots

When this fallback handles a field that was deliberately omitted from all_fielddescrs (for example an inline aggregate/nested struct, or a zero-sized layout-only field) but shares its byte offset with a real leaf, this offset-only search stamps the leaf's slot onto the missing field. That is exactly the found_parent_field == false case; downstream virtual materialization looks up fields by all_fielddescrs()[index_in_parent], so the aggregate/layout-only field can be emitted through the sibling leaf descriptor instead of staying unresolved. Only re-rank here when the parent entry also matches the field key/name; otherwise keep the unresolved fallback.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

github-actions · 2026-08-05T09:20:08Z

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit c98b31d).
Updated: 2026-08-05T20:24:12.733Z

Files in the reviewed diff

majit/majit-ir/src/descr.rs
majit/majit-macros/src/jit_interp/jitcode_lower/lower_vable.rs
majit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rs
majit/majit-metainterp/src/jitcode/assembler.rs
majit/majit-metainterp/src/pyjitpl/dispatch.rs
majit/majit-metainterp/src/trace_ctx.rs
majit/majit-translate/src/codewriter/assembler.rs
pyre/check.py
pyre/pyre-jit-trace/src/descr.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
pyre/pyre-jit-trace/src/jitcode_runtime.rs
pyre/pyre-jit/src/lib.rs
pyre/pyre-wasm-runner/src/main.rs
pyre/pyre-wasm/src/lib.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

None.

3. Pre-existing mismatches (already present before this patch)

None.

4. Structural adaptations

majit/majit-metainterp/src/jitcode/assembler.rs:5648 ↔ rpython/jit/backend/llsupport/descr.py:218 — pyre must finalize BhDescr::Field parents and indices after staged Rust bytecode emission; PyPy constructs the descriptor directly from one complete STRUCT. The new finalization restores the same field/parent pairing.
majit/majit-macros/src/jit_interp/jitcode_lower/lower_value.rs:586 ↔ rpython/jit/codewriter/heaptracker.py:97 — Rust lowering now carries stringify!(member) alongside offset/type ID. PyPy already receives fieldname; this is necessary because pyre’s emitted bytecode otherwise loses the name needed to distinguish flattened same-offset fields.
majit/majit-translate/src/codewriter/assembler.rs:3868 ↔ rpython/jit/codewriter/heaptracker.py:97 — the unique-offset fallback is a CPython-compatible compiler/layout adaptation for a field absent from pyre’s flattened field-name registry. It deliberately declines ambiguous offsets; PyPy’s low-level STRUCT representation instead resolves by field name directly.
majit/majit-ir/src/descr.rs:737 ↔ rpython/jit/backend/llsupport/descr.py:218 — producer-side atomic censuses have no PyPy runtime counterpart. They only detect violations before pyre’s cache reconciliation and do not alter descriptor semantics.
pyre/pyre-wasm/src/lib.rs:496 ↔ rpython/jit/codewriter/heaptracker.py:97 — wasm exports and runner-side stat collection are target-specific diagnostics required because wasm has no native stderr path; they do not change PyPy-equivalent JIT behavior.

coderabbitai · 2026-08-05T11:27:26Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

youknowone · 2026-08-05T11:27:41Z

Codex parity review — §1 and §2 confirmed and fixed

Both findings were about the same thing and both were real, so I measured
rather than argued: a temporary probe counting, per parent field list, how many
specs sit at an offset another field of the same parent also occupies.

[probe] spec_dup_offset=7      (of field_pos_spec_checked=1714)

Offset is not an identity. A flattened layout puts an inline aggregate and its
first leaf at one address (heaptracker.py:68-69 recurses into a nested
lltype.Struct without minting a descr for the container), so
position(|fd| fd.offset == offset) can name a sibling — exactly as the review
says.

What changed

field_slot_in (jitcode/assembler.rs) keys on the field name
(heaptracker.py:60-72 get_fielddescr_index_in(STRUCT, fieldname)) whenever the
descr carries one, and falls back to the byte offset only where there is no name
— and then only when exactly one field sits there. Where the offset is
ambiguous it resolves to nothing and the caller's number stands, the same
refusal descr.rs find_index_in_parent already documents.

Applied at all four sites the review named:

patch_field_descr_parents (§1) and its debug postcondition (§1, §2) — the
check is now stated over field_slot_in, so it can no longer panic on a descr
that names its field correctly and merely shares an address with a sibling.
fielddescrof's fallback arm and its census (§2) — unique_slot_at_offset.
Reaching that arm means the name lookup already missed, so offset is all there
is; declining on ambiguity is the only sound option.
field_descr_ref_from_bh's census (§2) — name first, unambiguous offset
otherwise. Counting an ambiguous one would have put a false reading behind
JITSTATS_BADNESS_FIELDS.

The pyre-jit-trace make_descr_from_bh census already keyed on
(offset, field_key), so it was unaffected.

§3 — one of the two pre-existing items is now closed

add_struct_field_descr has no field-name input and has always selected the
first matching offset.

Correct, and it was feeding a guessed name into the resolution above, which
is the damaging half: that name becomes the descr's _cache_field key
downstream, so naming the aggregate hands the leaf's access the aggregate's
descr. It now takes the documented (0, String::new()) fallback on an ambiguous
offset instead of the first match. The missing fieldname parameter itself is
still a real gap and is not something this PR closes.

The nested-struct flattening item (§3, second) is untouched — that is the same
rewrite_op_getsubstruct lowering the 152 field_pos_unresolved are waiting
on, and it is out of scope here.

Tests

an_ambiguous_offset_is_left_alone_rather_than_arbitrated — control:
arbitrating fails it (0, "agg") vs (0, "").
a_named_field_resolves_by_name_through_an_ambiguous_offset — control:
Some(1) vs None.

Verification

check.py dynasm 379/379, cranelift 379/379, wasm 375/375;
cargo test --workspace 101 suites, no failures. Counters unchanged and healthy
(field_pos_spec_misplaced=0, field_pos_attached_misplaced=0,
field_pos_unresolved=152).

The macos-latest `check.py` failure is not from this PR

Five max-pypy-ratio rows in the getattr/metatype family, three of which
check.py itself labels [pypy exec clamped to floor; ratio not a measurement].
main at 295b3fcad failed its macOS check.py with the same fixture, the
same 5.9x, the same clamped 0.01s denominator:

FAIL cranelift synth/metatype_property_dunder  exec 0.04s > pypy 0.01s  ratio 5.9x > gate 5x

The gated counters this PR adds read 0 on that CI run
(field_pos_spec_misplaced=0 field_pos_attached_misplaced=0).

— commented by Claude

youknowone · 2026-08-05T11:52:02Z

§1 and §2 are now `None`, and §3's first item is closed

The re-run reports no regressions and no new mismatches. §3's first item was
written against the previous push and is addressed by e7e04dde53c:

Pyre's add_struct_field_descr(offset, field_type, type_id) has no
fieldname … cannot select the field PyPy's name-keyed
heaptracker.get_fielddescr_index_in() selects.

The name was already there and simply not passed. jitcode_lower hands
register_struct_layout a (offset, is_ref, stringify!(#member)) triple and
then, in the same quote! expansion, called setfield_gc_* /
getfield_gc_* with the offset alone. Threading stringify!(#member) through
the five emitters makes read and write agree by construction and turns
field_slot_in into a real get_fielddescr_index_in(STRUCT, fieldname). The
unique-offset fallback stays for a caller that has no name to give.

an_ambiguous_offset_is_arbitrated_by_name_and_otherwise_declined now pins both
halves: the named access resolves to slot 1 ("leaf") through an offset two
fields share, the unnamed one still declines to (0, "").

The two remaining §3 items are deferred, with reasons

all_fielddescrs ordered by byte offset vs STRUCT._names traversal
order — a real ordering substitution, and the sharper of the two: it can
assign a different index_in_parent than upstream wherever declaration order
and offset order disagree, which no amount of name-keying fixes.
field_specs_from_layout sorts by offset deliberately (a Rust struct literal
has no stable source order across literals of the same type), so closing it
needs the declared field order carried from the front end — the same channel
the accessed-subset residual needs. Not something to bolt on inside this diff.
inline aggregate vs leaf at one address — this is the
jtransform.py:942 rewrite_op_getsubstruct lowering, the same one the 152
field_pos_unresolved are waiting on. Until a raw substruct access lowers to
int_add(ptr, offset) with no descr, an aggregate access has no exact
FieldDescr to be.

Verification

check.py dynasm 379/379, cranelift 379/379, wasm 375/375;
cargo test --workspace 101 suites, no failures. field_pos_* and
positional_* unchanged (spec_misplaced=0, attached_misplaced=0,
unresolved=152).

— commented by Claude

coderabbitai · 2026-08-05T17:55:16Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8feea75759

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-05T18:01:24Z

+        if !found_parent_field
+            && let Some(parent_spec) = parent.as_ref()
+            && let Some(pos) = unique_slot_at_offset(&parent_spec.all_fielddescrs, offset)
+        {
+            index_in_parent = pos;


Leave name-missing fields unresolved

Fresh evidence in this revision is that the new unique_slot_at_offset re-rank still runs after the parent-name lookup has failed, so a field intentionally omitted from all_fielddescrs but sharing an offset with a single leaf (for example a flattened inline aggregate or a zero-sized layout-only field) gets stamped with that leaf's slot. The resulting descr keeps the omitted field's key/offset but points index_in_parent at a sibling, and later virtual materialization indexes all_fielddescrs()[index_in_parent], emitting through the sibling descriptor instead of leaving the field unresolved; upstream derives this by field name and does not invent an offset match for nested structs/void fields. Please only re-rank when the parent entry also matches the field key/name.

AGENTS.md reference: AGENTS.md:L231-L232

Useful? React with 👍 / 👎.

`execute_new`'s result is already reachable: the dispatch arm stamps it onto the recorded op's `value` cell (`set_opref_concrete` → `history.py:803-807 *FrontendOp(pos, value)`) and `MetaInterp::walk_active_trace_refs` forwards every recorder `Op`/`InputArg` `value` holding a `Value::Ref`. Nothing between `bh_new` and that stamp allocates from the GC heap, so no collection can observe the object first. Record that contract on `execute_new_allocation` and on the two dispatch arms, and pin the stamp with `new_stamps_its_allocation_onto_the_recorded_op` / `new_with_vtable_...` — the walker already had a test, but nothing held the arms to stamping at all. A stub `Backend` overriding `bh_new` / `bh_new_with_vtable` exercises them; it needs real backing storage because `new_with_vtable` writes `w_class` into the block it returns. Also note on `FBW_FINISH_CONCRETE` why it is a genuine exception: the compile path does `self.tracing.take()`, so that stash outlives the op graph and is the one window no op-graph slot covers. Assisted-by: Claude

…e parent `add_struct_field_descr` derives `index_in_parent` and `name` by locating `offset` in the `struct_size_specs` entry as it stands at the mint site, and `register_struct_layout` re-indexes every entry on each merge, so a field minted before a lower-offset sibling is registered carries a rank the merge invalidates. `patch_field_descr_parents` already replaces that snapshot with the final merged spec, which leaves the descr's two halves describing different fields: the `descr.py:228` index points at a slot the `descr.py:238` parent fills with another offset. Re-resolve both against the spec being installed, using the mint's own `position(|fd| fd.offset == offset)` lookup over the complete list rather than a prefix of it. A field absent from the final spec keeps what the mint left it — those are the inline aggregates the flattened layout covers only through their leaves. `field_descr_index_follows_the_final_layout_not_the_mint_time_prefix` emits high-offset-first and asserts `all_fielddescrs[index_in_parent].offset` is the field itself; without the re-resolution it reports index 0 against a parent whose slot 0 is the other field. Measured on a hot two-field-instance loop: `field_pos_rederived` and `positional_misplaced` are 0 on both sides, so the runtime's by-name `derive_index_in_parent` was masking the stale index wherever the name resolved; `field_pos_unresolved` is 152 on both sides. `field_pos_parent_absent` 1210 -> 1201, `key_compared` 1210 -> 1201, `positional_slots` 1279 -> 1270, deterministic across runs. Assisted-by: Claude

`field_pos_rederived` counted disagreements found by `derive_index_in_parent`, which replaces the caller's `index_in_parent` in the same expression and only runs on the mint path with a published, non-empty parent whose field name resolves. `make_simple_descr_group_keyed_with_headerless` mints every field before `register_keyed_size` publishes the parent, so most mints take the `FIELD_PARENT_ABSENT` early return and are never compared. Add two censuses taken before that: * `field_pos_spec_{checked,misplaced}` — `field_specs[i].index_in_parent == i` on the list as handed to either group factory, covering the `cache_key == 0` groups that never enter `_cache_size`. * `field_pos_attached_{checked,misplaced}` — a standalone field descr's `index_in_parent` against the slot its own attached parent puts the offset in, counted at both `BhDescr::Field` readers. Both read 0 over the corpus (1713 and 248 checked), as do `positional_misplaced` and `field_pos_rederived`, with and without `patch_field_descr_parents`' index/name re-resolution — the ordering the pass fixes does not occur there. So add `field_descr_position_disagreement`, a `debug_assert!` postcondition over every emitted `Field` descr at `try_finish`, covering the resolved case, the unresolved `(0, "")` fallback and the descrs whose `type_id` the pass skips. Removing the two re-resolution lines trips it in `field_descr_index_follows_the_final_layout_not_the_mint_time_prefix`. Gate the two `misplaced` counters in `JITSTATS_BADNESS_FIELDS`; absent from a baseline they read 0, so no re-record. check.py dynasm 378/378, cargo test --workspace 101 suites, no failures. Assisted-by: Claude

…lback arms `fielddescrof`'s `struct_layout_for` and `heuristic_field_layout` arms overwrite `offset` from a second source and leave `index_in_parent` at its `0` initialiser, while `parent` still carries the whole flattened list — the descr names slot 0 of a list whose slot 0 is another field, and `all_fielddescrs()[index_in_parent]` is what `optimizeopt/info.rs force_box` indexes by. Resolve the slot against the attached parent by that offset. The `found_parent_field` arm is left alone: it picked its spec by name, so re-resolving by offset would return the wrong one for a struct with two fields at one offset. Census the same two halves at this producer, which stays meaningful after the fix — the `found_parent_field` arm takes the matched spec's stored `index_in_parent`, not its position in the list. `field_pos_attached_misplaced` reads 0 before and after over the corpus, so `fielddescrof_resolves_the_slot_when_the_offset_comes_from_the_layout_registry` constructs the state directly; removing the re-resolution fails it 0 vs 1. Replace the `debug_assert!` added in 6a5e644dd6d with a `cfg!(debug_assertions)` branch: the message needs the same walk as the predicate and `debug_assert!` ran it twice. check.py dynasm 378/378 and cranelift 378/378, cargo test --workspace 101 suites, no failures. Assisted-by: Claude

…wasm `JITSTATS_BADNESS_FIELDS` gained `field_pos_spec_misplaced` and `field_pos_attached_misplaced`, but the wasm guest has no stderr and never printed the `field_pos_*` line, so `_jit_stats_change` read both as absent and therefore zero — the healthy value. Export them the way the `descr_set_*` counters already are, with `pyre-wasm-runner` printing the line and failing the run when an export is missing. Not redundant with the native readings: the invariant is that `index_in_parent` names the slot the field's byte offset occupies, and wasm32 lays structs out on a 4-byte word (`symbolic.py:12 WORD = sizeof(lltype.Signed)`). The wasm run reports its own denominators — `field_pos_spec_checked=1665`, `field_pos_attached_checked=245` against 1712/247 on this host — so the exports measure the wasm-side descr universe rather than echoing the native one. `*_checked` are exported on the same footing as `pyre_jit_descr_set_resolved`: host-dependent, printed for diagnosis, absent from `JITSTATS_SNAPSHOT_FIELDS`. check.py wasm 374/374, dynasm 378/378, cargo test --workspace 101 suites, no failures. Assisted-by: Claude

…ous offset Codex parity review found the field-slot resolution keyed on byte offset where `heaptracker.py:60-72 get_fielddescr_index_in(STRUCT, fieldname)` keys on the name. Measured: 7 of 1714 submitted field specs sit at an offset another field of the same parent also occupies — a flattened inline aggregate and its first leaf share an address (`heaptracker.py:68-69`) — so offset is not an identity and `position(|fd| fd.offset == offset)` can name a sibling. `field_slot_in` resolves by name when the descr carries one and falls back to the offset only when it does not, and then only when exactly one field sits there. `add_struct_field_descr` is such a site: `getfield_gc_*` / `setfield_gc_*` reach it with `(offset, type, type_id)` and no `fieldname`. Where the offset is ambiguous it now takes the `(0, String::new())` fallback instead of the first match — a guessed name is the damaging half, since it becomes the descr's `_cache_field` key downstream. Same rule at the other three sites the review named: the codewriter's `fielddescrof` fallback arm (`unique_slot_at_offset`), and the two `census_attached_index` callers, which would otherwise report a correctly-named descr as misplaced and put a false reading behind `JITSTATS_BADNESS_FIELDS`. The `pyre-jit-trace` census already keyed on `(offset, field_key)`. Two tests: an ambiguous offset leaves the mint's number alone (control: arbitrating fails it `(0, "agg")` vs `(0, "")`), and a name resolves through an ambiguous offset (control: `Some(1)` vs `None`). check.py dynasm 379/379, cranelift 379/379, wasm 375/375; cargo test --workspace 101 suites, no failures. Assisted-by: Claude

`descr.py:218-239 get_field_descr(gccache, STRUCT, fieldname)` keys on the field name, and `heaptracker.py:60-72 get_fielddescr_index_in(STRUCT, fieldname)` derives the slot from it. `add_struct_field_descr` had no such argument and stood the byte offset in for it, which cannot select between two fields a flattened layout puts at one address. The name was already at the emit sites: `jitcode_lower` hands `register_struct_layout` a `(offset, is_ref, stringify!(#member))` triple and then called `setfield_gc_*` / `getfield_gc_*` with the offset alone, in the same `quote!` expansion. Thread it through the five emitters so read and write agree by construction. `field_slot_in` now resolves those descrs by name; the unique-offset fallback stays for a caller with no name to give. check.py dynasm 379/379, cranelift 379/379, wasm 375/375; cargo test --workspace 101 suites, no failures. `field_pos_*` and `positional_*` unchanged. Assisted-by: Claude

`field_slot_in` and `patch_field_descr_parents` both said `add_struct_field_descr` is handed `(offset, type, type_id)` and never sees a `fieldname`. It takes `field_name: &str`; every emit site supplies it — `jitcode_lower` passes the same `stringify!(#member)` it hands `register_struct_layout`, and `newlist_clear` passes the `"length"` / `"items"` it registered. The offset fallback is unchanged. What reaches it is a recorded name the final spec does not list, not a field that arrived anonymous. Assisted-by: Claude

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c98b31d811

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-05T20:15:18Z

+/// find_index_in_parent` documents for the runtime lookup, for the same reason.
+fn field_slot_in(fields: &[BhFieldSpec], name: &str, offset: usize) -> Option<usize> {
+    if !name.is_empty()
+        && let Some(idx) = fields.iter().position(|fd| fd.name == name)


Require matching offset for duplicate field names

When register_struct_layout has merged multiple layouts under one type_id, the list can contain the same field name at different offsets (the surrounding diagnostics track this as key_conflicting, e.g. the documented last_instr collision). This name-only lookup returns the first matching name even when the current descr’s offset is the later slot, and the new debug postcondition reuses the same helper without checking slot.offset, so it will bless an index_in_parent that points at a different field than the load/store offset. Please require the named slot to also match the descr offset, or treat duplicate names as unresolved/ambiguous.

Useful? React with 👍 / 👎.

15 synthetic fixtures move on all three backends: the arity-2 BUILD_TUPLE virtualization composes with the walker setfield_gc store and the FOR_ITER RETURN_VALUE admission from #1068 and the loop-perf folds from #1061, so the sre, exception-traceback and comprehension traces take fewer side exits — `nested_list_comprehension_hot` drops from 6 bridges / 1202 guard failures to 2 / 401, `sre_wasm_min` from 8 / 1849 to 5 / 1161. The 30 macro baselines only gain `field_pos_attached_misplaced` and `field_pos_spec_misplaced` at 0, the counters #1053 added to the binary without recording them here. Assisted-by: Claude

The baselines committed in e3d151e were recorded against a stale `build/llbc`: a `pyre-jit-trace` / `pyre-interpreter` edit invalidates the extraction fingerprint, and the JIT reads the function bodies it inlines out of those artefacts, so trace shape — not just field offsets — depends on them. The recorded counters therefore did not reproduce on CI, which extracts its own. `pyre/check.py (ubuntu-24.04)` failed with 49 jit-stats regressions across 19 benches on all three backends with identical numbers. Re-extracted `pyre-object pyre-interpreter pyre-jit`, rebuilt dynasm, cranelift and wasm with no `LLBC STALE` warning, and re-recorded. A local run now reproduces the CI numbers exactly, e.g. `nested_list_comprehension_hot` bridges 2 -> 6 and guard_failures 401 -> 1202. 84 counter values change across 21 benches (51 guard_failures, 33 bridges_compiled). 19 are the arity-2 tuple fold's mixed-representation side exits, which the ca7351f message under-reported for the same stale-artefact reason. Two are improvements from the specialised-pair subscript fold: `divmod_long_int_pair` guard_failures 9 -> 7 (its pair result now folds) and `exception_oserror_fields` 202 -> 201. The remaining 2218 added lines are `field_pos_attached_misplaced` / `field_pos_spec_misplaced`, counters #1053 added to the binary without recording them. check.py --synthetic-only: dynasm 371/371, cranelift 371/371, wasm 370/370. Assisted-by: Claude